Claude finished @Hmbown's task in 4m 17s —— View job

Code Review — PR #3481

• Gather context and understand the request
• Read changed files
• Analyze for correctness bugs and logic errors
• Check provider/model safety invariants
• Review for reuse/simplification opportunities
• Assess test coverage
• Check security concerns
• Post final review

Overall: The extraction into facts-lib.mjs is the right move and the diffFacts test suite is solid. There is one correctness bug that makes the CI drift gate a no-op, a behavioral divergence between the build-time and runtime sandbox derivation, and a stale guidance string left in facts-drift.ts. No provider-inference safety violations.

[Bug — CI gate is a no-op]

.github/workflows/web.yml lines 36–44:

- name: Generate derived facts
  run: npm run prebuild          # ← OVERWRITES facts.generated.ts
- name: Check facts drift
  run: npm run check:facts       # ← compares freshly-written file to freshly-derived facts

prebuild runs derive-facts.mjs, which regenerates facts.generated.ts from the current workspace and writes it to disk. check:facts then reads that freshly-written file and compares it to the current workspace. They will always match — the committed version is never compared. If a developer bumps Cargo.toml but forgets to run prebuild and commit the updated facts.generated.ts, CI will silently pass.

Fix: swap the two steps so the drift check reads the committed file before prebuild overwrites it.

- name: Check facts drift        # sees the committed file
  run: npm run check:facts
- name: Generate derived facts   # regenerates for tsc
  run: npm run prebuild

Fix this →

[Moderate — deriveSandboxBackends diverges between build-time and runtime]

facts-lib.mjs:46-52 returns all unrecognised sandbox filenames as-is (after the exclusion list):

const map = { seatbelt: "seatbelt (macOS)", landlock: "landlock (Linux)" };
return files.map((f) => map[f] ?? f);   // bwrap, process_hardening, seccomp pass through

facts-drift.ts:126-135 only keeps names that appear in its own map:

return files.filter((n) => map[n]).sort().map((n) => map[n]);  // only seatbelt + landlock

The committed facts.generated.ts therefore has five backends (bwrap, landlock (Linux), process_hardening, seatbelt (macOS), seccomp), but when the Cloudflare cron runs it will derive only two (landlock (Linux), seatbelt (macOS)) and write that smaller list to KV, overriding the build-time value. The website would then show an incomplete sandbox list.

Either align facts-drift.ts to pass through unmapped names, or explicitly exclude bwrap/process_hardening/seccomp in facts-lib.mjs the same way facts-drift.ts implicitly does.

[Minor — stale guidance string in facts-drift.ts:115]

facts-drift.ts line 115 still says:

"Add them to labelMap here AND in web/scripts/derive-facts.mjs …"

Since this PR moved the authoritative map to facts-lib.mjs, that message should now point to web/scripts/facts-lib.mjs. A developer hitting this warning will look in the wrong file.

Fix this →

[Minor — parseCommittedFacts regex is fragile] (check-facts.mjs:44)

const m = src.match(/export const FACTS\s*:\s*\w+\s*=\s*([\s\S]*?);?\s*$/);

This works for the current generated format, but would silently fail to parse if: (a) a second export is added after FACTS, (b) as const appears before the ;, or (c) the type annotation spans more than one word (\w+ won't match RepoFacts & Foo). Since derive-facts.mjs fully controls the output format it's low-risk today, but worth a comment or a more deliberate extraction strategy (e.g., strip the header lines and JSON-parse the remainder).

Good choices

• Extracting derivation logic into facts-lib.mjs so derive-facts.mjs and check-facts.mjs share one implementation is correct — any future bug in regex or path logic is fixed in one place.
• The diffFacts test matrix (version, providers, tool count, null-to-value, multi-field, ignored fields) covers the meaningful cases and would have caught regressions.
• The unmapped-variant warning in deriveProviders ensures a new enum variant in config.rs can't silently drop from the provider list.
• facts.generated.ts being removed from .gitignore is the right call — without it the committed file can never be verified against the workspace.